feat: tron batch payments contracts and tests#1724
Conversation
Greptile SummaryThis PR ports the existing EVM
Confidence Score: 5/5The contracts are safe to merge; both findings are test quality and error-message clarity issues, not fund-loss or logic bugs. Both contracts behave correctly under all tested scenarios. The allowance check omission leads to a less helpful revert message rather than wrong behaviour, and the missing confirmation waits affect test reliability but not contract correctness. packages/smart-contracts/test/tron/ERC20BatchPayments.test.js and packages/smart-contracts/tron/contracts/BatchPayments.sol Important Files Changed
Reviews (5): Last reviewed commit: "chore: remove contract ownership" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const expectRevertOrNoBalanceChange = async (fn, getBalances) => { | ||
| const before = await getBalances(); | ||
| try { | ||
| await fn(); | ||
| } catch (_error) {} | ||
| await waitForConfirmation(2000); | ||
| const after = await getBalances(); | ||
| const unchanged = before.every((value, index) => value === after[index]); | ||
| return { unchanged }; | ||
| }; |
There was a problem hiding this comment.
expectRevertOrNoBalanceChange catches errors without asserting that a revert actually occurred. If the transaction unexpectedly succeeds and the balance check happens to track the wrong account, a funds leak would be silently accepted. The expectNonOwnerReverts helper in the same file correctly asserts threw === true; applying the same pattern here would make failure detection explicit.
There was a problem hiding this comment.
On Tron, when a revert occurs the transaction call does not fail.
See this transaction for example.
|
@LeoSlrRf is this still true? If not, please update the PR description. > Currently, both contracts provide methods for managing the underlying proxies. |
|
@MantisClone It wasn't the case. I just pushed the ownership removal and updated the PR description |
| uint256[] calldata _amounts, | ||
| bytes[] calldata _paymentReferences, | ||
| uint256[] calldata _feeAmounts, | ||
| address _feeAddress |
There was a problem hiding this comment.
I noticed that all batch payment contracts enforce a single feeAddress for all payments. I don't see why that would be the case.
Given that we won't use the fee parameters, I'm wondering if it's worth updating to an array for Tron only.
2 participants
Description of the changes
Adds Tron-specific batch payment contracts and test infrastructure to the
smart-contractspackage.This PR introduces the existing EVM
BatchPaymentcontract on Tron, and introducesERC20BatchPayments,a version for better performance and increased security on TronTwo Tron-targeted test suites are added:
ERC20BatchPayments.test.jscoversERC20BatchPayments, including happy-path single-token and multi-token payments, zero-amount payments, zero-fee payments,BadTRC20token handling, and error cases such as insufficient funds, missing allowance, and mismatched input arrays.BatchPayments.test.jscovers the mainBatchPaymentscontract on Tron, adding batch fee computation and verification, dynamic batch fee updates, admin access control for proxy and fee setters, and a check thatbatchEthPaymentsWithReferencereverts when noEthFeeProxyis configured.A shared
helpers.jsmodule provides utilities used by both test files, including token deployment, approval helpers, balance diffing, batch fee computation, and revert/no-balance-change assertion helpers.The root
package.jsonworkspace configuration is updated to prevent hoisting of@openzeppelindependencies for thesmart-contractspackage, ensuring the Tron build resolves its own copy of those contracts.Contracts Scope
BatchPayments.sol
Same contract as the one on EVM.
We don't have the EthFeeProxy on Tron, so the associated address is defined as the zero address. Calls to the associated method fail as expected.
ERC20BatchPayment.sol
This is the same contract as above, with existing
BatchPayments without:EthFeeProxymethodsSecurity Consideration
The
BatchPaymentcontract provides methods for managing the underlying proxies and fees. And exposes an unused method related to native payment.On the other hand, the
ERC20BatchPaymentcontract benefits from not exposing unused methods, thereby reducing the attack surface. Additionally, since we don't need an owner to manage the fees, the ERC20FeeProxy contract is now hardcoded, which allows us to remove the contract owner.Gas Consideration
ERC20BatchPaymenthas a lower gas footprint because it does not include batch-fee-related logic.Initially, I created a third version in which the
feeAmountsandfeeRecipientswere removed from the interface and were always set to zero within the contract. This didn't reduce the gas cost compared to a call where we would pass the zero values directly.